listgeo: fix corner coordinates for images with RasterPixelIsPoint #36
Conversation
| @@ -227,20 +226,29 @@ static void GTIFPrintCorners( GTIF *gtif, GTIFDefn *defn, FILE * fp_out, | |||
|
|
|||
| { | |||
| printf( "\nCorner Coordinates:\n" ); | |||
|
|
|||
| unsigned short raster_type = RasterPixelIsArea; | |||
| GTIFKeyGet(gtif, GTRasterTypeGeoKey, &raster_type, 0, 1); | |||
There was a problem hiding this comment.
I am not entirely pleased with this solution to obtain the value of the key, as it ignores the return value and doesn't check for any undesired values in raster_type. Should one be strict here?
There was a problem hiding this comment.
Can you use GTIFKeyGetSHORT() instead of GTIFKeyGet() ? This is safer in case of corrupted files.
Your current logic otherwise looks reasonable to me.
There was a problem hiding this comment.
Can you use GTIFKeyGetSHORT() instead of GTIFKeyGet() ?
Unfortunately GTIFKeyGetSHORT and related functionality is declared static in geo_normalize.c, so not without further changes.
There was a problem hiding this comment.
Good point. I think it could be worth exporting GTIFKeyGetSHORT() and GTIFKeyGetDOUBLE() in geotiff.h
There was a problem hiding this comment.
Sounds like a good idea, but should probably go into another PR?
There was a problem hiding this comment.
I'm fine if it goes into that PR as a separate commit. By the way if you rebase your PR on top of latest master, the CI failure on Mac should now be resolved
There was a problem hiding this comment.
Rebased, will look into exposing the mentioned functionality next.
| @@ -227,20 +226,29 @@ static void GTIFPrintCorners( GTIF *gtif, GTIFDefn *defn, FILE * fp_out, | |||
|
|
|||
| { | |||
| printf( "\nCorner Coordinates:\n" ); | |||
|
|
|||
| unsigned short raster_type = RasterPixelIsArea; | |||
| GTIFKeyGet(gtif, GTRasterTypeGeoKey, &raster_type, 0, 1); | |||
There was a problem hiding this comment.
Can you use GTIFKeyGetSHORT() instead of GTIFKeyGet() ? This is safer in case of corrupted files.
Your current logic otherwise looks reasonable to me.
… in corner coordinates
There is now a relatively minimal test-geotiff included that makes it easy to tell whether the behaviour is as intended. Is this sufficient? I am also not sure whether I understand the current test-setup correctly, see the failing travis "build". Am I correcctly reading that the only failing thest is the one that compares between this and 1.5.1 because of the additional test case? |
yes, if you look at the log at https://travis-ci.com/github/OSGeo/libgeotiff/jobs/319144984 , the likely cause is that you forgot to git add & commit the new test file |
|
@CaptainCarrot Could you commit the missing file so this PR passes ? I'd like to do a release this week |
|
@rouault Sorry, I have been busy with other matters. The test (./testlistgeo) works locally (silly excuse I know), and I think I have extended the test correctly here: 3d7b04d The CI failure here https://travis-ci.com/github/OSGeo/libgeotiff/jobs/319144984 seems to come from the fact that there is a new test case and image, which are not present in 1.5.1? |
ah, I see now you committed the file, but I know what's wrong: you should add in EXTRA_DIST in test/Makefile.am, since Travis tests on the tarball generated by "make dist", and without that the file, will not be added to it |
This is a proposal to resolve the issue raised in #35, where the corner coordinates reported by listgeo are off by half a pixel if GTRasterTypeGeoKey is set to RasterPixelIsPoint instead of RasterPixelIsArea.
Currently there are no tests that use RasterPixelIsPoint, as they're RasterPixelIsArea exclusively. Should I one (or multiple)?